Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit 'connecting' event from PQ handshake #1006

Conversation

matszczygiel
Copy link
Contributor

@matszczygiel matszczygiel commented Dec 2, 2024

Problem

When issuing a PQ connection request, the node state was reported as 'CONNECTING' only when the PQ handshake was finished. This led to no state being reported when the handshake was failing.

Solution

Emit a 'CONNECTING' event upon PQ handshake

To Reviewers

The approach taken here is, to some extent, naive. But unfortunately there is some time pressure to fix the event emiting issue, due to a fact that integrators are, to some extent, blocked by the bug. Therefore fixing of the issue has been divided into two parts, one is implement - maybe not the most clean solution, but quick one to avoid unecessary development blockages. And then refactor this solution into better approach. The quick-and-dirty solution is being implemented in this PR, and the refactoring is being performed here: #1021.

☑️ Definition of Done checklist

  • Commit history is clean (requirements)
  • README.md is updated
  • Functionality is covered by unit or integration tests

@matszczygiel matszczygiel force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from 322a6b6 to db6e3e7 Compare December 2, 2024 13:32
@matszczygiel matszczygiel force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from db6e3e7 to 33cf89d Compare December 2, 2024 14:31
@matszczygiel matszczygiel force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from 33cf89d to af0152c Compare December 3, 2024 09:29
@matszczygiel matszczygiel force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from af0152c to 0da9ad5 Compare December 3, 2024 09:29
@matszczygiel matszczygiel force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from 0da9ad5 to a7d82ca Compare December 3, 2024 10:23
@matszczygiel matszczygiel force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from a7d82ca to 3087a62 Compare December 3, 2024 10:30
@matszczygiel matszczygiel marked this pull request as ready for review December 3, 2024 11:42
@matszczygiel matszczygiel requested a review from a team as a code owner December 3, 2024 11:42
@Jauler Jauler force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from 8229ed0 to 6b8bcff Compare December 12, 2024 10:48
@Jauler Jauler force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from 6b8bcff to 9ff41f7 Compare December 12, 2024 10:52
@Jauler Jauler force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from 9ff41f7 to 5592c18 Compare December 12, 2024 10:56
@Jauler Jauler force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from 5592c18 to a60d8bc Compare December 12, 2024 11:02
@Jauler Jauler force-pushed the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch from a60d8bc to 74d027a Compare December 12, 2024 12:09
src/device.rs Show resolved Hide resolved
src/device.rs Show resolved Hide resolved
src/device.rs Show resolved Hide resolved
Copy link
Contributor

@LukasPukenis LukasPukenis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am missing natlab testcase which checks the happy path where peer receives all expected events when correctly connecting.

I see there's test_pq_vpn_silent_pq_upgrader however that tests the non-pq server - what about the happy case?

@Jauler
Copy link
Contributor

Jauler commented Dec 12, 2024

I am missing natlab testcase which checks the happy path where peer receives all expected events when correctly connecting.

I see there's test_pq_vpn_silent_pq_upgrader however that tests the non-pq server - what about the happy case?

Every test using _connect_to_vpn_pq is enforcing that connecting events are coming, and disconnect_from_vpn() in telio.py is waiting for disconnect event. Hence the happy path test is test_pq_vpn_connection(). And a test which reproduces the issue (e.g. does not pass without fix) is test_pq_vpn_silent_pq_upgrader().

Copy link
Contributor

@LukasPukenis LukasPukenis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@mathiaspeters mathiaspeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

A couple of comments I would like to see fixed in the follow-up PR, but this is good to go in my opinion

@Jauler Jauler merged commit a47c971 into main Dec 12, 2024
64 checks passed
@Jauler Jauler deleted the msz/LLT-5814-post-quantum-vpn-status-is-reported-only-after-post-quantum-handshake branch December 12, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants